Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Property filter enum tokens #2739

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pan-kot
Copy link
Member

@pan-kot pan-kot commented Sep 19, 2024

Description

Adds first-class support for multi-choice (enum) tokens in property filter.

Rel: [Tz3OAr3i2ni1], [0aVwAjMOimcN]

Depends on:

How has this been tested?

  • New unit tests
  • Screenshot tests
Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.22%. Comparing base (cc75463) to head (dd8a730).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2739   +/-   ##
=======================================
  Coverage   96.21%   96.22%           
=======================================
  Files         761      761           
  Lines       21502    21556   +54     
  Branches     7354     7326   -28     
=======================================
+ Hits        20688    20742   +54     
+ Misses        806      761   -45     
- Partials        8       53   +45     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pan-kot pan-kot force-pushed the feat-property-filter-enum-props branch 3 times, most recently from 9e32565 to ec57f06 Compare September 20, 2024 08:28
@pan-kot pan-kot changed the title poc: Property filter enum props POC chore: Property filter enum props POC Sep 20, 2024
@pan-kot pan-kot force-pushed the feat-property-filter-enum-props branch from ec57f06 to af7c2bf Compare September 20, 2024 12:16
@pan-kot pan-kot force-pushed the feat-property-filter-enum-props branch from af7c2bf to 128fbd4 Compare September 24, 2024 10:18
@pan-kot pan-kot force-pushed the feat-property-filter-enum-props branch from 128fbd4 to 9853c8a Compare October 7, 2024 12:27
@pan-kot pan-kot force-pushed the feat-property-filter-enum-props branch from 9853c8a to 085c7b9 Compare October 7, 2024 13:12
@pan-kot pan-kot force-pushed the feat-property-filter-enum-props branch 3 times, most recently from f592e4f to d16f901 Compare October 8, 2024 16:00
@pan-kot pan-kot force-pushed the feat-property-filter-enum-props branch from d16f901 to 3d8604c Compare October 9, 2024 16:52
@pan-kot pan-kot force-pushed the feat-property-filter-enum-props branch from 3d8604c to 680473e Compare October 10, 2024 08:14
@pan-kot pan-kot force-pushed the feat-property-filter-enum-props branch 2 times, most recently from 9b9c3dd to a8f6987 Compare October 10, 2024 11:06
@pan-kot pan-kot force-pushed the feat-property-filter-enum-props branch 2 times, most recently from 0fbd924 to 10eff20 Compare October 11, 2024 10:21
@pan-kot pan-kot force-pushed the feat-property-filter-enum-props branch from 10eff20 to f0b493a Compare October 11, 2024 11:31
@@ -230,9 +230,7 @@ const InternalButtonDropdown = React.forwardRef(
{text}
</InternalButton>
);
trigger = showMainActionOnly ? (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is needed so that onKeyDown and onKeyUp handlers are attached that prevent Enter keypress propagation. Without that, the main action cannot be activated with Enter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The listeners are attached to the outer div instead of the button dropdown? Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure, but I think that is because those should work for both the trigger and the dropdown: if the listeners are added to the button dropdown trigger alone, that might change the current behaviour. I can experiment with that but in a standalone PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this fixing an existing bug? If so, can we add coverage for the fix? I'd be OK with having this in a separate PR (actually even better).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a separate PR: #2897

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also created a separate one for embedded multiselect fixes: #2896

@pan-kot pan-kot force-pushed the feat-property-filter-enum-props branch from 27195c3 to 92ef6ba Compare October 17, 2024 09:37
@pan-kot pan-kot changed the title chore: Property filter enum props POC feat: Property filter enum tokens Oct 17, 2024
@pan-kot pan-kot marked this pull request as ready for review October 17, 2024 09:41
@pan-kot pan-kot requested a review from a team as a code owner October 17, 2024 09:41
@pan-kot pan-kot requested review from jperals and removed request for a team October 17, 2024 09:41
@pan-kot pan-kot force-pushed the feat-property-filter-enum-props branch from 92ef6ba to 1107f2c Compare October 17, 2024 09:54
pages/property-filter/common-props.tsx Show resolved Hide resolved
src/multiselect/embedded.tsx Show resolved Hide resolved
@@ -230,9 +230,7 @@ const InternalButtonDropdown = React.forwardRef(
{text}
</InternalButton>
);
trigger = showMainActionOnly ? (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The listeners are attached to the outer div instead of the button dropdown? Why?

@@ -12636,7 +12636,7 @@ The \`operation\` property has two valid values: "and", "or", and controls the j
The \`tokens\` property is an array of objects that will be displayed to the user beneath the filtering input. When \`enableTokenGroups=true\`, the
\`tokenGroups\` property is used instead, which supports nested tokens.
Each token has the following properties:
* value [string]: The string value of the token to be used as a filter.
* value [unknown]: The value of the token to be used as a filter. Can be null or string for default tokens, string[] for enum tokens, and anything for tokens with custom forms.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double-check that this API change is OK (the doc text update suggests yes), and this will not cause issues to existing customers (do we want to run a dry run?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did make a dry-run for the type update in collection hooks. In this PR no type was updated, but will do a dry-run as well, just in case.

The value has been essentially unknown since introduction of custom forms. The current PR should have no impact to the existing customers but those making something with the query and wanting to adopt the feature would need to ensure the type is correctly handled at runtime.

};
};

test('formats tokens using custom and default formatters', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we mean by "custom and default formatters"? Are we indeed testing both?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, the enum tokens are formatted as value.join(', '). It is also possible to provide a custom format function. In the test page this is done for State and Tags properties, in one case to translate options and in another case to truncate long values, if more than 5 options are selected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants